-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Force exit on DYNDNS server failure #29
Force exit on DYNDNS server failure #29
Conversation
@britter Can you check whether this resolves the situation? |
I've applied this change to my homelab, see britter/nix-configuration@6abaa4d. This looks better in the sense that the program exists now in case of bind errors. However it exists with 0 exit code. I think a non-zero exit code would be more appropriate in that situation. |
@@ -154,7 +165,7 @@ func startPushServer(out chan<- *net.IP, localIp *net.IP) { | |||
|
|||
go func() { | |||
err := s.ListenAndServe() | |||
slog.Error("Server stopped", logging.ErrorAttr(err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the error logging worked better here, where the log can be more specific. Or maybe log in both places? Once for the error once for stopping from that error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reason moving it was to allow for other things to use the context as well, the cause must be an error
so it needs a log anyway if the context gets "done".
We could wrap the ListenAndServe()
error and escalate it, if you want the additional information, like
go func() {
err := s.ListenAndServe()
cancel(errors.Join(errors.New("http server error"), err))
}()
but I'm fine with any approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that could make sense, so the error source isn't lost.
Maybe this way. It's not the prettiest output with slog
but contains everything and could be |
@britter can you check again that it properly exits now? After that I'll merge it and if the other improvements are there as well, I'll also see that there's a release published. |
The latest changes look great to me. When using this app as a systemd unit it's not correctly exiting with non-zero exit code causing it to go into state Thanks a ton for adding this so quickly! |
Possible fix for #26, only tested with
DYNDNS_SERVER_BIND
set, everything else disabled. Should not affect the other related services.